Skip to content

Validate CoinPay CLI payment arguments - #629

Merged
ralyodio merged 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/coinpay-payment-validation
Jun 6, 2026
Merged

Validate CoinPay CLI payment arguments#629
ralyodio merged 1 commit into
profullstack:masterfrom
rissrice2105-agent:codex/coinpay-payment-validation

Conversation

@rissrice2105-agent

Copy link
Copy Markdown
Contributor

Fixes #628.

Changes:

  • validate CoinPay create amount as a positive number
  • validate optional businessId before adding it to CLI args
  • normalize and validate blockchain/coin/fiat asset codes
  • require paymentId for get commands
  • add tests proving invalid configs do not call the external CoinPay CLI

Validation:

  • vitest run packages/targets/payment-coinpay/src/index.test.ts
  • tsc -p packages/targets/payment-coinpay/tsconfig.json --noEmit

@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds pre-flight validation to the CoinPay CLI target adapter, catching bad arguments (non-positive amounts, blank IDs, malformed asset codes) before the external coinpay CLI is ever invoked, and normalises coin/fiat/blockchain codes to uppercase. It also removes requiredSecrets (an unused property on TargetContractOptions) and adds a focused Vitest suite covering each validation path.

  • Input guards added: requirePositiveAmount, requireText, optionalText, and requireAssetCode cover all four command handlers; the requireAssetCode helper also applies a strict ^[A-Z0-9_]{2,16}$ format check after normalisation.
  • Test coverage: six new test cases exercise the happy path for create and rates plus all four rejection paths (zero amount, blank businessId, whitespace-only paymentId, and malformed coin code).

Confidence Score: 4/5

Safe to merge; the validation logic is correct for all normal inputs and the tests prove the key rejection paths.

The validation helpers work correctly and the test suite covers the main paths. The only notable gap is that requireAssetCode uses === undefined for its fallback guard rather than nullish coalescing, so an explicit null coin/fiat bypasses the default and surfaces a misleading required error.

packages/targets/payment-coinpay/src/index.ts — specifically the requireAssetCode fallback and the optional blockchain field in the create handler.

Important Files Changed

Filename Overview
packages/targets/payment-coinpay/src/index.ts Adds input validation helpers (requireText, optionalText, requirePositiveAmount, requireAssetCode) and wires them into all four command handlers; minor issue with null vs undefined handling in requireAssetCode fallback logic.
packages/targets/payment-coinpay/src/index.test.ts Adds targeted unit tests covering positive validation, amount rejection, blank businessId, missing paymentId, asset code normalisation, and malformed code rejection; mock setup and cleanup look correct.

Reviews (1): Last reviewed commit: "Validate CoinPay CLI payment arguments" | Re-trigger Greptile

}

function requireAssetCode(value: unknown, name: string, fallback?: string): string {
const raw = value === undefined ? fallback : value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The fallback in requireAssetCode only fires when value === undefined, which means explicitly passing null (a valid unknown value in Record<string, unknown>) bypasses the default and falls through to requireText(null, name), which throws "coin required" instead of using the fallback. The more conventional JavaScript idiom for "treat null and undefined the same" is nullish coalescing, which would correctly apply the fallback for both.

Suggested change
const raw = value === undefined ? fallback : value;
const raw = value ?? fallback;

if (config.args?.blockchain) args.push('--blockchain', String(config.args.blockchain));
const amount = requirePositiveAmount(config.args?.amount);
args.push('--amount', String(amount));
if (config.args?.blockchain) args.push('--blockchain', requireAssetCode(config.args.blockchain, 'blockchain'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing blockchain field validation for createblockchain is optional here but may be required by the CoinPay CLI. If omitted, the CLI invocation goes out without --blockchain and any resulting error surfaces as an opaque CLI failure rather than an early, clear validation message. Consider making the field required for create (or documenting that the CLI has a default).

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

9 similar comments
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: git fetch upstream master && git rebase upstream/master.

@ralyodio
ralyodio merged commit 422165e into profullstack:master Jun 6, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

payment-coinpay accepts invalid CLI payment arguments

2 participants